fix select/archive #368
fix select/archive #368
Conversation
WalkthroughRefactors Spectr root discovery by extracting downward traversal, Git-root detection, and directory-skip heuristics into new modules; reduces maxDiscoveryDepth from 10 to 5; adds a mutex-protected per-working-directory cache and helpers in Changes
Sequence Diagram(s)mermaid Caller->>CmdCache: GetDiscoveredRoots(cwd) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/discovery/roots.go (1)
237-238:⚠️ Potential issue | 🔴 CriticalBug:
filepath.SplitListsplits PATH-style strings, not path components.
filepath.SplitListis designed for splitting environment variables likePATH(colon/semicolon separated), not for counting path depth. This will return incorrect results—typically1for any relative path—causing incorrect sorting.Use
strings.Countwithfilepath.Separatoror iterate withfilepath.Splitinstead.Proposed fix
import ( "fmt" "os" "path/filepath" "sort" + "strings" )- // Compare by number of path separators (shorter = closer) - // "." has 0 separators (closest) - // ".." has 1 separator - // "../.." has 2 separators, etc. - depthI := len(filepath.SplitList(relI)) - depthJ := len(filepath.SplitList(relJ)) + // Compare by number of path components (fewer = closer) + // "." has 1 component (closest) + // ".." has 1 component + // "../.." has 2 components, etc. + depthI := strings.Count(relI, string(filepath.Separator)) + 1 + depthJ := strings.Count(relJ, string(filepath.Separator)) + 1
🤖 Fix all issues with AI agents
In `@internal/discovery/downward.go`:
- Around line 96-98: The boolean expression "info.IsDir() || !info.IsDir()" is
always true, so in the return inside downward.go replace the redundant test with
a simple existence check: return err == nil; in other words, remove the
"(info.IsDir() || !info.IsDir())" part and rely on the error from os.Stat (the
variable err) to determine whether ".git" exists (using the existing info
variable).
- Around line 46-49: The code incorrectly uses filepath.SplitList to count
directory components; replace that with splitting the cleaned relative path on
the OS path separator: after computing relPath with filepath.Rel (and handling
relErr), call filepath.Clean(relPath) and then split on string(os.PathSeparator)
(or use strings.Count+1 semantics) to compute the component count; update the
code references around relPath/relErr and add imports for strings and os if
needed.
In `@internal/discovery/skip_dirs.go`:
- Line 25: The "*.egg-info" key in the skipDirs map is ineffective because
literal map lookup won't match directories like "mypkg.egg-info"; either remove
the "*.egg-info" entry from the skipDirs map or update shouldSkipDirectory to
check suffixes (e.g., strings.HasSuffix(name, ".egg-info")) in addition to
existing exact and glob checks so directories ending with ".egg-info" are
correctly skipped; reference the skipDirs map entry "*.egg-info" and the
shouldSkipDirectory function when making the change.
🧹 Nitpick comments (3)
GEMINI.md (1)
39-39: Minor: Add language specifier to fenced code block.The static analysis tool flagged this code block as missing a language specifier. Consider adding a language identifier for proper syntax highlighting.
-``` +```text cmd/ # CLI commands (thin layer using Kong framework)cmd/discovery.go (1)
44-47: Consider edge case: nil slice vs empty slice.If
FindSpectrRootsreturns anilslice (rather than an empty slice[]SpectrRoot{}), the cache checkcachedRoots != nilwill be false, causing repeated discovery calls. Depending onFindSpectrRoots's behavior, consider explicitly marking cache validity:+ cacheValid boolHowever, reviewing
FindSpectrRoots, it appears to always return a non-nil slice on success, so this is likely fine in practice.internal/discovery/git_root.go (1)
52-60: Consider simplifying the.gitexistence check.The two conditions on lines 53-55 and 58-60 can be combined since both return
currentwhenerr == nil. The current structure is correct but verbose.Proposed simplification
func findGitRootUncached(startPath string) string { current := startPath for { gitDir := filepath.Join(current, gitDirName) - info, err := os.Stat(gitDir) - if err == nil && info.IsDir() { - return current - } - - // Also check for git worktree files (where .git is a file, not dir) - if err == nil && !info.IsDir() { + // Check for .git directory or worktree file + if _, err := os.Stat(gitDir); err == nil { return current } parent := filepath.Dir(current)
| relPath, relErr := filepath.Rel(absStartPath, path) | ||
| if relErr == nil { | ||
| return len(filepath.SplitList(relPath)) | ||
| } |
There was a problem hiding this comment.
Bug: filepath.SplitList splits PATH-style strings, not path components.
Same issue as in roots.go: filepath.SplitList is for environment variables like PATH, not for counting directory depth. This will produce incorrect depth calculations.
Proposed fix
+import "strings" // Fallback: calculate depth from path segments
relPath, relErr := filepath.Rel(absStartPath, path)
if relErr == nil {
- return len(filepath.SplitList(relPath))
+ if relPath == "." {
+ return 0
+ }
+ return strings.Count(relPath, string(filepath.Separator)) + 1
}🤖 Prompt for AI Agents
In `@internal/discovery/downward.go` around lines 46 - 49, The code incorrectly
uses filepath.SplitList to count directory components; replace that with
splitting the cleaned relative path on the OS path separator: after computing
relPath with filepath.Rel (and handling relErr), call filepath.Clean(relPath)
and then split on string(os.PathSeparator) (or use strings.Count+1 semantics) to
compute the component count; update the code references around relPath/relErr
and add imports for strings and os if needed.
| ".tox": {}, | ||
| ".nox": {}, | ||
| ".eggs": {}, | ||
| "*.egg-info": {}, |
There was a problem hiding this comment.
Glob pattern *.egg-info won't match actual directories.
The map uses literal string keys, so "*.egg-info" will only match a directory literally named *.egg-info, not directories like mypackage.egg-info. Consider removing this entry or implementing suffix matching in shouldSkipDirectory.
Proposed fix
Option 1: Remove the ineffective entry:
- "*.egg-info": {},Option 2: Add suffix check in shouldSkipDirectory:
func shouldSkipDirectory(dirName string) bool {
// Fast path: check the pre-computed set
if _, skip := skipDirsSet[dirName]; skip {
return true
}
+ // Check for .egg-info suffix (Python package metadata)
+ if len(dirName) > 9 && dirName[len(dirName)-9:] == ".egg-info" {
+ return true
+ }
+
// Skip hidden directories (except .git which is handled separately)
if len(dirName) > 1 && dirName[0] == '.' && dirName != gitDirName {
return true
}
return false
}🤖 Prompt for AI Agents
In `@internal/discovery/skip_dirs.go` at line 25, The "*.egg-info" key in the
skipDirs map is ineffective because literal map lookup won't match directories
like "mypkg.egg-info"; either remove the "*.egg-info" entry from the skipDirs
map or update shouldSkipDirectory to check suffixes (e.g.,
strings.HasSuffix(name, ".egg-info")) in addition to existing exact and glob
checks so directories ending with ".egg-info" are correctly skipped; reference
the skipDirs map entry "*.egg-info" and the shouldSkipDirectory function when
making the change.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation